-
Notifications
You must be signed in to change notification settings - Fork 610
add support for DRAExtendedResources #3629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
2f250e1 to
45252c2
Compare
45252c2 to
7cef5a6
Compare
|
Discussed offline. And we want to reconcile this with existing DRA test folder to reduce duplication. |
| # Make sure a Prometheus stack is deployed | ||
| ./run-e2e.sh cluster-loader2 \ | ||
| --provider=kind \ | ||
| --kubeconfig=/root/.kube/config \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? Don't think everyone uses root user to run their cluster.
| ```bash | ||
| # Make sure a Prometheus stack is deployed | ||
| ./run-e2e.sh cluster-loader2 \ | ||
| --provider=kind \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my experience with load scenario default kind cluster configuration doesn't not fulfill some prerequisites (e.g. metric endpoints of KCM/scheduler are not exposed). Have you verified that it works?
| - kube-scheduler | ||
| - kubelet | ||
|
|
||
| 2. **DRA Driver**: A DRA driver must be running (installed automatically by the test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's installed, then it's not a prerequesite? Same for Prometheus.
| --nodes=5 | ||
| ``` | ||
|
|
||
| ## Test Flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the purpose of the following documentation.
| parallelism: {{.Replicas}} | ||
| completions: {{.Replicas}} | ||
| completionMode: {{.Mode}} | ||
| activeDeadlineSeconds: 86400 # 24 hours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need two manifests that differ by 3 lines?
| Timeout: 5m | ||
|
|
||
| steps: | ||
| - name: Start measurements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not move monitoring to submodule?
|
The change looks safe from CL2 perspective, please address Marek's comments before submitting. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alaypatel07, mborsz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Alay Patel <[email protected]>
7cef5a6 to
db99097
Compare
|
@mengqiy @serathius I have consolidated both the dra and dra-extended-resources test into one. PTAL @serathius can I take the using measurements as submodule as a followup? It will help me land this within the deadline |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds support for extended resources feature in DRA dependency as well as add testing manifests for extended resource feature.
Which issue(s) this PR fixes:
Fixes kubernetes/kubernetes#133757
Special notes for your reviewer:
This PR is blocked by the fixes here: kubernetes/kubernetes#134312